Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Custom serialization #61

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

danieroux
Copy link

This implements #60

I made the configs first-class keys, and not using kafka/consumer-opts. Consumer-opts feels more like "whatever this plugin did not consider".

Copy link
Member

@lbradstreet lbradstreet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks good to merge once my minor comments are addressed.

@@ -79,10 +79,20 @@
:default 2000
:optional? true}

:kafka/deserializer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:kafka/deserializer -> :kafka/value-deserializer for consistency with kafka.

Copy link
Author

@danieroux danieroux Nov 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one is a tricky one for me.

I was keeping it consistent with :kafka/deserializer-fn - if you change the :kafka/deserializer you have to consider how it affects your :kafka/deserializer-fnchoice.

I changed the documentation to say something about it.

Do you still want me to change it to be consistent with kafka?

src/onyx/kafka/information_model.cljc Outdated Show resolved Hide resolved
…ray is expected

- Also assert in the test that the key/value in those functions are what we expect
- Preserve take-now signature before custom serializers were added
- Allow end-offsets and beginning-end-offsets-clj to be used with custom serializers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants